-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SqlProtocolTcpIp: Auto-detect the TCP/IP address group name by IP address #1940
SqlProtocolTcpIp: Auto-detect the TCP/IP address group name by IP address #1940
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1940 +/- ##
====================================
- Coverage 91% 91% -1%
====================================
Files 92 92
Lines 7806 7891 +85
====================================
+ Hits 7181 7251 +70
- Misses 625 640 +15
|
Please see comment in issue. |
Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again. |
Closing this as it has been no response on the issue #1939. Will keep the issue open for a while longer. This PR can be re-opened if need be after the discussion in the issue, and the missing tests that need to be contributed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reopened and have reviewed.
Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status: 1 of 4 files reviewed, 8 unresolved discussions (waiting on @claudiospizzi and @github-code-scanning[bot])
a discussion (no related file):
The unit tests need to be added that tests this change.
source/DSCResources/DSC_SqlProtocolTcpIp/DSC_SqlProtocolTcpIp.psm1
line 545 at r1 (raw file):
$IpAddressGroup = Find-IpAddressGroup -IpAddressGroup $IpAddressGroup -InstanceName $InstanceName $IpAddressGroup = Convert-IpAdressGroupCasing -IpAddressGroup $IpAddressGroup
This should not be needed in the Test-TargetResource function since it calls Compare-function that calls Get-function. Get-function will sort this out. 🤔
Code quote:
$IpAddressGroup = Find-IpAddressGroup -IpAddressGroup $IpAddressGroup -InstanceName $InstanceName
$IpAddressGroup = Convert-IpAdressGroupCasing -IpAddressGroup $IpAddressGroup
source/DSCResources/DSC_SqlProtocolTcpIp/DSC_SqlProtocolTcpIp.psm1
line 770 at r1 (raw file):
The IP address already stored in an IP address group. #> function Find-IpAddressGroup
We should make this a public command Find--SqlDscManagedComputerServerProtocolIpAddressGroup
. See other comments as well.
source/DSCResources/DSC_SqlProtocolTcpIp/DSC_SqlProtocolTcpIp.psm1
line 782 at r1 (raw file):
[Parameter(Mandatory = $true)] [AllowEmptyString()]
The IP Group can never be empty so this should be removed.
Code quote:
[AllowEmptyString()]
source/DSCResources/DSC_SqlProtocolTcpIp/DSC_SqlProtocolTcpIp.psm1
line 796 at r1 (raw file):
) Import-SqlDscPreferredModule
This should not be needed as it done when SqlServerDsc is imported (which will be done when the public command is used). We should be able to remove this. See next comment too.
Code quote:
Import-SqlDscPreferredModule
source/DSCResources/DSC_SqlProtocolTcpIp/DSC_SqlProtocolTcpIp.psm1
line 808 at r1 (raw file):
} $serverProtocolProperties = Get-ServerProtocolObject @getServerProtocolObjectParameters
We should make this a public command Get-SqlDscManagedComputerServerProtocol
(that also resuses Get-SqlDscManagedComputer
). See command Get-SqlDscManagedComputerService
for example how the new command should work. The command Get-ServerProtocolObject
can be kept also and be switched to the new public command in other PR's.
Code quote:
Get-ServerProtocolObject
Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again. |
Closing this as there have been no more progress on this. Please reopen to continue working on this, or another contributor can open a new PR. |
Pull Request (PR) description
Currently, it's not possible to detect an TCP/IP address group by using the IP. The TCP/IP address group name is mandatory during compilation time. For SQL Server running multiple instances with multiple per-instance IP's, it's important to define con compile time, which IPx address group belongs to which TCP/IP address.
This Pull Request (PR) fixes the following issues
The resource should support entering the IP address in the group name field, so that the group is detected by the IP address.
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
This change is